Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support SDM Subscription and Unsubscription for UE Session #123

Merged
merged 5 commits into from
Oct 29, 2024

Conversation

saileshvvr
Copy link
Contributor

@saileshvvr saileshvvr commented Sep 14, 2024

Description

  • Add support for SDM Subscription during UE 1st PDU Session Establishment
  • Add support for SDM UnSubscription during release of last UE PDU Session

@andy89923 andy89923 self-assigned this Sep 14, 2024
@saileshvvr saileshvvr force-pushed the udm_sdm_subscription_support branch 2 times, most recently from 479c658 to 3650397 Compare September 14, 2024 17:15
@ianchen0119
Copy link
Contributor

@andy89923
Please help to test the functionality (w/ OAuth and w/o OAuth)

@andy89923 andy89923 changed the title SMF to support SDM Subscription and Unsubscription for UE Session feat: support SDM Subscription and Unsubscription for UE Session Sep 19, 2024
internal/sbi/consumer/udm_service.go Outdated Show resolved Hide resolved
internal/sbi/consumer/udm_service.go Outdated Show resolved Hide resolved
@saileshvvr
Copy link
Contributor Author

Hi @andy89923 ,
Addressed the comment. I did not follow the refactored code and used the legacy way.
Updated as suggested :)
Thanks!

@saileshvvr
Copy link
Contributor Author

Hi @andy89923
Can you please share if in case anything missing.
As this PR functionality is pending from quite sometime, would like to check once!
Thanks

Copy link
Contributor

@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saileshvvr Sorry for the late review!

After testing w/wo OAuth, this PR can subscribe when a new UE and unsubscribe when the UE's last PDU session is released.

Thanks for the PR, LGTM!

@@ -121,6 +121,17 @@ func (p *Processor) HandlePDUSessionSMContextCreate(
}
}

if !p.Context().Ues.UeExists(smContext.Supi) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the error happens after the SDM data changed?
I think use the defer would be better in this case:

defer func(err error) {
  if err != nil {
	if !p.Context().Ues.UeExists(smContext.Supi) {
		if problemDetails, err := p.Consumer().
			Subscribe(ctx, smContext, smPlmnID); problemDetails != nil {
			smContext.Log.Errorln("SDM Subscription Failed Problem:", problemDetails)
		} else if err != nil {
			smContext.Log.Errorln("SDM Subscription Error:", err)
		}
	} else {
		p.Context().Ues.IncrementPduSessionCount(smContext.Supi)
	}
  }
}(err)

internal/sbi/processor/pdu_session.go Show resolved Hide resolved
internal/sbi/processor/pdu_session.go Show resolved Hide resolved
internal/sbi/processor/pdu_session.go Show resolved Hide resolved
@ianchen0119
Copy link
Contributor

It looks good to me except the requested changes.

Hi @saileshvvr
Please help to update & test the PR based on my comments.
Many thanks!

@saileshvvr
Copy link
Contributor Author

Hi @ianchen0119 ,

Thank you for the feedback. As I was occupied with other work, missed the comments.
I shall check and update accordingly.

Thanks!

@andy89923
Copy link
Contributor

Hi @saileshvvr ,

We can't use the defer function and pass the error to differentiate if the error happens after the defer function.
Should creat an error instead.

func main() {
	var err error
	defer func() {
		if err != nil {
			fmt.Printf("defer: %s\n", err)
		} else {
			fmt.Println("defer: defer not error")
		}
	}()

	err = errors.New("new error 1")
	if err != nil {
		return
	}
}

/*output
defer: new error 1
*/

You can refer to https://stackoverflow.com/questions/42703707/when-defer-func-evaluates-its-parameters

@saileshvvr
Copy link
Contributor Author

Hi @andy89923 , @ianchen0119 ,
Agree with your comments. Can we handle this as a seperate improvement PR once after delivering this implementation.
After submitting this PR I shall raise a new improvement PR with the recommended changes if you are OKAY.
please share your comments. Thanks!

@andy89923
Copy link
Contributor

Hi @saileshvvr ,

Can we handle this as a separate improvement PR?

No, we should revise this in the same PR.

If you don't have time, I will help revise if the PR isn't ready until next Wednesday.

@saileshvvr
Copy link
Contributor Author

saileshvvr commented Oct 6, 2024

Thank you @andy89923 .
As I shared earlier, I was occupied with other tasks and may not be getting time during next week.
Thanks for your quick response and support.

@andy89923
Copy link
Contributor

Hi @saileshvvr ,

Sorry, I don't have time to help with this PR; please fix the comments when you are free!

@saileshvvr
Copy link
Contributor Author

Hi @ianchen0119 , @andy89923 ,
As I understand we can keep the code with in defer{}() only during Subscribe().

For all other cases (Session Release(UE/NW triggered) or Local Release or Failure cases) we can just call Unsubscribe(). As Unsubscribe() will decrement the session count if more than 1 session else session HTTP DELETE to Unsubscribe.
So no change may be needed during Unsubscribe() call.

Please let me know your comments so as to update the latest patch accordingly.

Thanks
Sailesh

@saileshvvr
Copy link
Contributor Author

saileshvvr commented Oct 29, 2024

Thanks @ianchen0119 @andy89923 for uploading the latest changes.
Please let me know if the final changes in the PR is ok to be delivered.

@ianchen0119 ianchen0119 merged commit 2d87e36 into free5gc:main Oct 29, 2024
3 checks passed
@saileshvvr saileshvvr deleted the udm_sdm_subscription_support branch October 30, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants